Skip to content

[neighorch]: Avoid inserting missing bulk neighbors#4772

Open
Xichen96 wants to merge 1 commit into
sonic-net:masterfrom
Xichen96:dev/xichenlin/neighorch-enable-neighbor-lookup
Open

[neighorch]: Avoid inserting missing bulk neighbors#4772
Xichen96 wants to merge 1 commit into
sonic-net:masterfrom
Xichen96:dev/xichenlin/neighorch-enable-neighbor-lookup

Conversation

@Xichen96

@Xichen96 Xichen96 commented Jul 20, 2026

Copy link
Copy Markdown

What I did

Prevented NeighOrch::enableNeighbors() from inserting a default cache entry while checking whether a bulk-enable neighbor exists.

Root cause

The old code read the map before checking it:

ctx->mac = m_syncdNeighbors[neighborEntry].mac;

if (m_syncdNeighbors.find(neighborEntry) == m_syncdNeighbors.end())
{
    continue;
}

std::map::operator[] inserts a default value when the key is missing. The intended absence check was therefore unreachable:

missing neighbor
    -> operator[] inserts a default entry
    -> find() succeeds
    -> default MAC/state enters the bulk-enable path

The fix uses one non-mutating lookup and reads the MAC only after the entry is confirmed:

auto neighborIt = m_syncdNeighbors.find(neighborEntry);
if (neighborIt == m_syncdNeighbors.end())
{
    continue;
}
ctx->mac = neighborIt->second.mac;

Why I did it

MuxNbrHandler::enable() can hand NeighOrch a context for a neighbor that disappeared from m_syncdNeighbors before bulk processing. A read-side guard must not manufacture the state it is trying to detect.

This defect was found while reviewing PR #4746, but it predates that change and belongs in a separate PR because it affects NeighOrch cache correctness and MUX bulk-enable behavior, not the IntfsOrch removal fence.

The fix has no standalone master merge dependency and remains independently reviewable.

How I verified it

Added EnableNeighborsDoesNotInsertMissingNeighbor in tests/mock_tests/neighorch_ut.cpp.

The regression:

  1. creates a bulk context for a neighbor absent from m_syncdNeighbors;
  2. verifies the cache is empty;
  3. calls enableNeighbors();
  4. verifies the intended missing-neighbor no-op succeeds;
  5. verifies the cache remains empty.

Current exact-head master PR build

  • Head: faa3e6df99028137237ea70c9fa99f5a8b8c097c
  • Azure PR build: 1171074
  • All architecture, ASAN, Docker, Trixie, Test vstest, and TestAsan vstest lanes passed.
  • CodeQL, Semgrep, DCO, EasyCLA, and the exact-head Copilot review are clean.
  • No local build was used; validation is PR-build only.

202605 PR-build validation

Details if related

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Copilot AI review requested due to automatic review settings July 20, 2026 17:27
@Xichen96
Xichen96 requested a review from prsunny as a code owner July 20, 2026 17:27
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a subtle cache-mutation bug in NeighOrch::enableNeighbors() where reading m_syncdNeighbors[neighborEntry] could insert a default-constructed neighbor entry, making subsequent “not found” handling ineffective. The change ensures missing neighbors are skipped without altering the cache and adds a mock regression test to prevent reintroduction.

Changes:

  • Replace operator[] access with a find()-based lookup before reading a neighbor’s cached MAC in enableNeighbors().
  • Skip processing for missing neighbor entries without mutating m_syncdNeighbors.
  • Add a mock unit test verifying enableNeighbors() does not insert a missing neighbor entry.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
orchagent/neighorch.cpp Avoids unintended insertion into m_syncdNeighbors by using find() before reading cached neighbor data in enableNeighbors().
tests/mock_tests/neighorch_ut.cpp Adds regression coverage ensuring a missing neighbor in a bulk enable request does not create a cache entry.

@Xichen96

Copy link
Copy Markdown
Author

/azpw run

@mssonicbld

Copy link
Copy Markdown
Collaborator

⚠️ Notice: /azpw run only runs failed jobs now. If you want to trigger a whole pipline run, please rebase your branch or close and reopen the PR.
💡 Tip: You can also use /azpw retry to retry failed jobs directly.

Retrying failed(or canceled) jobs...

@mssonicbld

Copy link
Copy Markdown
Collaborator

Retrying failed(or canceled) stages in build 1170080:

✅Stage TestAsan:

  • Job vstest: retried.

✅Stage Test:

  • Job vstest: retried.
  • Job vstest: retried.

@Xichen96 Xichen96 closed this Jul 21, 2026
@Xichen96 Xichen96 reopened this Jul 21, 2026
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@mssonicbld

Copy link
Copy Markdown
Collaborator

This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description.

---Powered by SONiC BuildBot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants